Add ability to specify idbag (#415) #755
Conversation
jeff-hagen
changed the title
|
@hazzik would you mind taking a look at my PR? |
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to specify an idbag collection type in FluentNHibernate mappings. Idbag is a type of collection mapping in NHibernate that combines characteristics of a bag with an identifier column for each row. The implementation includes mapping configuration, XML generation, and visitor pattern support.
- Adds
Collection.IdBagenum value and associated mapping infrastructure - Implements
CollectionIdMappingclass to configure the collection identifier - Adds
AsIdBag()methods to fluent mapping API with configurable identifier type and generation strategy
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Collection.cs | Adds IdBag enum value to collection types |
| CollectionMapping.cs | Adds support for CollectionId property and IdBag factory method |
| CollectionIdMapping.cs | New mapping class for collection identifier configuration |
| ToManyBase.cs | Adds fluent AsIdBag() methods for configuring idbag collections |
| XmlIdBagWriter.cs | New XML writer for generating idbag elements |
| XmlCollectionIdWriter.cs | New XML writer for collection-id elements |
| XmlCollectionWriter.cs | Updates collection writer to handle IdBag type |
| Visitor classes | Adds support for visiting CollectionIdMapping |
| GeneratorBuilder.cs | Refactors default generator logic into reusable method |
| CollectionIdPart.cs | New fluent configuration class for collection identifiers |
| Test files | Adds comprehensive tests for idbag functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/FluentNHibernate/MappingModel/Collections/CollectionIdMapping.cs
Outdated
Show resolved
Hide resolved
src/FluentNHibernate/MappingModel/Collections/CollectionMapping.cs
Outdated
Show resolved
Hide resolved
|
@hazzik I addressed all of the review issues. Is this OK to get merged? |
|
Bump. Can someone look at this MR? |
| if (identityType != typeof(string)) throw new InvalidOperationException("Identity type must be string"); | ||
| } | ||
|
|
||
| internal void SetDefault() |
There was a problem hiding this comment.
@jeff-hagen sorry for the delay in reviewing. Can you move this change to a separate PR?
| public class XmlMapWriter(IXmlWriterServiceLocator serviceLocator) | ||
| : BaseXmlCollectionWriter(serviceLocator), IXmlWriter<CollectionMapping> | ||
| { | ||
| readonly IXmlWriterServiceLocator serviceLocator = serviceLocator; |
There was a problem hiding this comment.
@jeff-hagen could you please move the optimizations of service locator fields from hierarchy to a separate PR?
There was a problem hiding this comment.
@hazzik But this is an integral part of the PR. The rationale behind refactoring it is to prevent from having to copy / paste the original set default code. That would be an anti-pattern? What am I missing?
There was a problem hiding this comment.
Would be better to put this into another PR as then it can be merged earlier.
|
Sorry for the delay coming back to you. I tried to review this PR several times, but have not had time to finish it to the state I wanted. One option that is causing delays and the one I really want to explore is to try treating id-bag, from a modelling perspective, the same way as other indexed collections (eg array, list, indexed-many-to...). So the id-bag will become just a bag with an |
|
But idbag does not support one-to-many like bag does. The only difference from a mapping perspective is HasMany vs HasManyToMany, but the latter is much more representative of the fact that idbag does not support one-to-many. Using HasMany would be "lying." Please help me understand. |
2 participants
Closes #415
Adds the ability to specify a collection .AsIdBag(). Note that the NHibernate xml mapping model is a little odd: